Skip to content

fix: PLI-19(위시리스트 피드백 수정)#90

Merged
kimbosung521 merged 11 commits intodevelopfrom
feature-PLI-19
Apr 10, 2026
Merged

fix: PLI-19(위시리스트 피드백 수정)#90
kimbosung521 merged 11 commits intodevelopfrom
feature-PLI-19

Conversation

@kimbosung521
Copy link
Copy Markdown
Contributor

  • 실시간 추천 시 바텀시트 높이 조절
  • 모달 수정
  • 위시 리시트 하트 기본 값 수정
  • 검색 일회용 수정
  • 컴포넌트 분리

- 실시간 추천 시 바텀시트 높이 조절
- 모달 수정
- 위시 리시트 하트 기본 값 수정
- 검색 일회용 수정
@kimbosung521
Copy link
Copy Markdown
Contributor Author

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the WishlistScreen by modularizing its components into separate files and improving state management for liked items across different tabs. It also introduces a maxTopSnap property to CustomBottomSheet for better control over snap points. Feedback was provided regarding the nesting of interactive elements in the WishlistSearchBar, which could lead to accessibility and touch event propagation issues.

Comment on lines +379 to +387
<WishlistSearchBar
searchInputRef={searchInputRef}
searchQuery={searchQuery}
onChangeText={setSearchQuery}
onFocus={handleSearchFocus}
onBlur={handleSearchInputBlur}
onFocusInput={focusSearchInput}
onPressBack={handleGoBack}
/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The WishlistSearchBar component is rendered as a child of a Pressable that calls onFocusInput. This nesting of interactive elements (a Pressable wrapping a TextInput and other TouchableOpacity components) can lead to accessibility issues and unexpected touch event propagation behavior on mobile platforms. Consider moving the onPress handler to the WishlistSearchBar component itself or using a more standard layout.

@takjinwu
Copy link
Copy Markdown
Contributor

takjinwu commented Apr 8, 2026

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the WishlistScreen by modularizing its components, including the bottom sheet, search bar, and search overlay, and introduces new tab-specific components (WishTabTrending, WishTabSave, WishTabWishlist). It also improves the bottom sheet's snap behavior and adds support for maxTopSnap. My feedback highlights a logic issue in the search overlay where the 'like' functionality is hardcoded to the 'wishlist' tab, and suggests enabling the 'like' button in the 'trending' tab to improve user experience.

Comment on lines +426 to +432
<WishlistSearchOverlay
isVisible={isSearchFocused}
selectedCategory={selectedCategory}
places={SEARCH_PLACES}
isLiked={(id) => isLikedInTab('wishlist', id)}
onToggleLike={(id) => toggleLike('wishlist', id)}
/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

검색 오버레이(WishlistSearchOverlay)에서 '좋아요' 기능이 'wishlist' 탭에 하드코딩되어 있습니다. 사용자가 '저장된 장소' 탭에서 검색을 시작한 경우에도 검색 결과의 '좋아요' 버튼은 '위시리스트'에만 영향을 주어 사용자에게 혼란을 줄 수 있습니다.

현재 선택된 탭(selectedCategory)에 따라 '좋아요' 기능이 동적으로 작동하도록 수정하는 것을 고려해 보세요. 예를 들어, selectedCategory가 'trending'이 아닐 때 해당 탭의 '좋아요' 목록에 추가/삭제되도록 할 수 있습니다.

Suggested change
<WishlistSearchOverlay
isVisible={isSearchFocused}
selectedCategory={selectedCategory}
places={SEARCH_PLACES}
isLiked={(id) => isLikedInTab('wishlist', id)}
onToggleLike={(id) => toggleLike('wishlist', id)}
/>
<WishlistSearchOverlay
isVisible={isSearchFocused}
selectedCategory={selectedCategory}
places={SEARCH_PLACES}
isLiked={(id) => isLikedInTab(selectedCategory === 'trending' ? 'wishlist' : selectedCategory, id)}
onToggleLike={(id) => toggleLike(selectedCategory === 'trending' ? 'wishlist' : selectedCategory, id)}
/>


// FlatList renderItem → 반드시 별도 컴포넌트 + memo (스크롤 성능)
const TrendingPlaceItem = React.memo<{ item: WishPlace }>(({ item }) => (
<PlaceCard place={item} isLiked={false} onToggleLike={() => {}} isTrending={true} />
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

'실시간 추천' 탭의 장소 카드에 있는 VectorIcon 버튼에 onToggleLike prop으로 빈 함수 () => {}가 전달되어, 버튼이 클릭 가능해 보이지만 실제로는 아무런 동작도 하지 않습니다. 이는 사용자에게 혼란을 줄 수 있습니다.

이 버튼에 의도된 기능(예: 위시리스트에 추가)을 연결하는 것을 권장합니다. 이를 위해 WishTabTrending 컴포넌트가 onToggleLike 함수를 prop으로 받아 PlaceCard에 전달하도록 수정할 수 있습니다.

1. types.ts 수정:
WishTabTrendingPropsonToggleLike을 추가합니다.

export interface WishTabTrendingProps {
  places: WishPlace[];
  onToggleLike: (id: string) => void;
}

2. WishTabTrending.tsx 수정:
prop을 받아 PlaceCard에 전달합니다.

// line 8
<PlaceCard place={item} isLiked={false} onToggleLike={onToggleLike} isTrending={true} />

// line 12
export const WishTabTrending = React.memo<WishTabTrendingProps>(({ places, onToggleLike }) => {
//...

3. WishlistScreen.tsx 수정:
WishTabTrending 컴포넌트에 onToggleLike 핸들러를 전달합니다.

// line 329
return <WishTabTrending places={TRENDING_PLACES} onToggleLike={(id) => toggleLike('wishlist', id)} />;

이렇게 수정하면 '실시간 추천' 목록의 항목을 바로 위시리스트에 추가할 수 있어 사용자 편의성이 향상됩니다.

@takjinwu
Copy link
Copy Markdown
Contributor

takjinwu commented Apr 8, 2026

/gemini review

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the WishlistScreen by modularizing components into dedicated files and types, and introduces new UI components like WishlistBottomSheet, WishlistSearchBar, and WishlistSearchOverlay. While the modularization is a positive step, I have identified several areas for improvement: the useAnimatedReaction logic in WishlistScreen appears redundant due to existing constraints in CustomBottomSheet, the WishModal contains an invalid Tailwind class, and the use of FlatList with scrollEnabled={false} inside ScrollView is inefficient for small lists and should be replaced with .map(). Additionally, I recommend standardizing the animation library by using react-native-reanimated in WishlistSearchOverlay for consistency.

Comment thread src/screens/WishlistScreen.tsx Outdated
Comment on lines +278 to +286
useAnimatedReaction(
() => ({ y: translateY.value, isTrending: isTrendingTabSV.value }),
({ y, isTrending }, prev) => {
const isMovingUp = y < (prev?.y ?? y);
if (isTrending && isMovingUp && y < SNAP_TRENDING) {
translateY.value = SNAP_TRENDING;
}
},
);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

useAnimatedReaction을 통한 위치 제한 로직은 중복으로 보입니다. CustomBottomSheet 컴포넌트 내부에서 이미 maxTopSnap 프로퍼티를 통해 translateY의 최소값(SNAP_TOP_LIMIT)을 제한하고 있기 때문에, UI 스레드에서 별도의 반응형 로직으로 값을 강제할 필요가 없습니다. 또한 이를 위해 사용된 isTrendingTabSV 공유 변수와 관련 useEffect 로직도 함께 제거하여 코드를 단순화할 수 있습니다.


{/* X 버튼 */}
{showCloseButton ? (
<Pressable onPress={onClose} className="absolute top-4 right-4 x-6 y-6">
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

x-6 y-6 클래스는 표준 Tailwind CSS 클래스가 아닙니다. 아이콘의 크기나 영역을 설정하려 했다면 w-6 h-6 또는 p-1 등의 표준 클래스로 수정하는 것이 적절합니다.

Suggested change
<Pressable onPress={onClose} className="absolute top-4 right-4 x-6 y-6">
<Pressable onPress={onClose} className="absolute top-4 right-4 w-6 h-6">

Comment on lines +46 to +56
return (
<FlatList
data={places}
renderItem={renderItem}
keyExtractor={keyExtractor}
scrollEnabled={false}
removeClippedSubviews={true}
className="py-4 mr-[1px]"
/>
);
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

ScrollView 내부에서 scrollEnabled={false}FlatList를 사용하는 것은 가상화(Virtualization) 기능을 비활성화하여 모든 아이템을 한 번에 렌더링하게 만듭니다. 리스트가 길어질 경우 성능 저하를 유발할 수 있으므로, 아이템 수가 적은 경우 오버헤드가 적은 .map() 방식을 사용하는 것이 더 효율적입니다.

  return (
    <View className="py-4 mr-[1px]">
      {places.map((item) => (
        <SavePlaceItem
          key={`saved-${item.id}`}
          item={item}
          isLiked={isLiked(item.id)}
          onToggleLike={onToggleLike}
        />
      ))}
    </View>
  );

Comment on lines +25 to +32
<FlatList
data={places}
renderItem={renderItem}
keyExtractor={keyExtractor}
scrollEnabled={false}
removeClippedSubviews={true}
className="py-4 mr-[1px]"
/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

FlatListscrollEnabled={false}로 사용하는 대신 .map()을 사용하여 불필요한 컴포넌트 오버헤드를 줄이는 것이 좋습니다.

Suggested change
<FlatList
data={places}
renderItem={renderItem}
keyExtractor={keyExtractor}
scrollEnabled={false}
removeClippedSubviews={true}
className="py-4 mr-[1px]"
/>
<View className="py-4 mr-[1px]">
{places.map((item) => (
<TrendingPlaceItem key={`trending-${item.id}`} item={item} />
))}
</View>

Comment on lines +47 to +54
<FlatList
data={places}
renderItem={renderItem}
keyExtractor={keyExtractor}
scrollEnabled={false}
removeClippedSubviews={true}
className="py-4 mr-[1px]"
/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

FlatListscrollEnabled={false}로 사용하는 대신 .map()을 사용하여 불필요한 컴포넌트 오버헤드를 줄이는 것이 좋습니다.

      <View className="py-4 mr-[1px]">
        {places.map((item) => (
          <WishlistPlaceItem
            key={`wishlist-${item.id}`}
            item={item}
            isLiked={isLiked(item.id)}
            onToggleLike={onToggleLike}
          />
        ))}
      </View>

Comment on lines +1 to +58
import React from 'react';
import { Animated, View } from 'react-native';
import PlaceCard from './PlaceCard';
import type { PlaceCardProps, WishlistBottomSheetTabId } from '@/screens/wishList/components';
import { ScrollView } from 'react-native-gesture-handler';

interface WishlistSearchOverlayProps {
isVisible: boolean;
selectedCategory: WishlistBottomSheetTabId;
places: PlaceCardProps['place'][];
isLiked: (id: string) => boolean;
onToggleLike: (id: string) => void;
}

export const WishlistSearchOverlay = React.memo<WishlistSearchOverlayProps>(
({ isVisible, selectedCategory, places, isLiked, onToggleLike }) => {
const animatedOpacity = React.useRef(new Animated.Value(isVisible ? 1 : 0)).current;

React.useEffect(() => {
Animated.timing(animatedOpacity, {
toValue: isVisible ? 1 : 0,
duration: 180,
useNativeDriver: true,
}).start();
}, [animatedOpacity, isVisible]);

return (
<Animated.View
pointerEvents={isVisible ? 'auto' : 'none'}
style={{
position: 'absolute',
top: 0,
left: 0,
right: 0,
bottom: 0,
zIndex: 40,
backgroundColor: 'white',
opacity: animatedOpacity,
}}>
<ScrollView>
<View className="px-4">
<View className="h-14 mb-3" />
{places.map((place) => (
<PlaceCard
key={`${selectedCategory}-${place.id}`}
place={place}
isLiked={isLiked(place.id)}
onToggleLike={onToggleLike}
/>
))}
</View>
</ScrollView>
</Animated.View>
);
},
);

WishlistSearchOverlay.displayName = 'WishlistSearchOverlay';
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

프로젝트의 다른 부분에서는 react-native-reanimated를 사용하여 애니메이션을 처리하고 있으나, 이 컴포넌트에서는 react-native 기본 Animated 라이브러리를 사용하고 있습니다. 일관성을 유지하고 Reanimated의 성능 이점(UI 스레드 실행 등)을 활용하기 위해 useAnimatedStyle 등을 사용하는 방식으로 통일하는 것을 권장합니다.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the WishlistScreen by modularizing its components into smaller, reusable parts such as WishlistBottomSheet, WishlistSearchBar, and WishlistSearchOverlay. It also introduces a dedicated types.ts file for better type management and improves the bottom sheet's snap behavior with a new maxTopSnap property. The review comments suggest ensuring the absolute positioning of the search bar is managed robustly as the UI grows and verifying that the renderTabContent prop passed to the bottom sheet is stable to avoid unnecessary re-renders.

Comment on lines +379 to +387
<WishlistSearchBar
searchInputRef={searchInputRef}
searchQuery={searchQuery}
onChangeText={setSearchQuery}
onFocus={handleSearchFocus}
onBlur={handleSearchInputBlur}
onFocusInput={focusSearchInput}
onPressBack={handleGoBack}
/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The WishlistSearchBar component is being used as a direct child of the SafeAreaView's inner View but is positioned absolutely. While this works, it is better to ensure that the WishlistSearchBar does not overlap with other UI elements unexpectedly by managing its z-index and layout more explicitly if the screen complexity grows.

Comment on lines +415 to +424
<WishlistBottomSheet
translateY={translateY}
onStateChange={handleSheetChange}
maxTopSnap={selectedCategory === 'trending' ? SNAP_TRENDING : 0}
tabs={TABS}
selectedCategory={selectedCategory}
onSelectCategory={setSelectedCategory}
onPressComplete={handleComplete}
renderTabContent={renderTabContent}
/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The WishlistBottomSheet component is being rendered conditionally based on selectedCategory. Ensure that the renderTabContent prop is memoized or stable to prevent unnecessary re-renders of the bottom sheet content when the parent component state updates.

@kimbosung521 kimbosung521 merged commit d2c993d into develop Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants